feat(api): dataset read API uuid support #34836
Conversation
github-actions
bot
added
api
dankor
force-pushed
the
uuid-get-dataset
branch
from
5318afa to
c4cceef
Compare
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/daos/exceptions.py | ✅ |
| superset/daos/datasource.py | ✅ |
| superset/views/datasource/utils.py | ✅ |
| superset/common/query_object_factory.py | ✅ |
| superset/common/query_context_factory.py | ✅ |
| superset/daos/base.py | ✅ |
| superset/datasets/api.py | ✅ |
| superset/charts/schemas.py | ✅ |
| superset/utils/core.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
dankor
changed the title
dankor
force-pushed
the
uuid-get-dataset
branch
2 times, most recently
from
c03a7db to
f970fe4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #34836 +/- ##
===========================================
+ Coverage 0 72.36% +72.36%
===========================================
Files 0 584 +584
Lines 0 42542 +42542
Branches 0 4485 +4485
===========================================
+ Hits 0 30784 +30784
- Misses 0 10574 +10574
- Partials 0 1184 +1184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dankor
force-pushed
the
uuid-get-dataset
branch
4 times, most recently
from
1d8a009 to
998a12b
Compare
dankor
changed the title
|
@mistercrunch @michael-s-molina @rusackas |
dankor
force-pushed
the
uuid-get-dataset
branch
2 times, most recently
from
a0b72a0 to
fa3d2e9
Compare
michael-s-molina
requested review from
Vitor-Avila,
betodealmeida,
michael-s-molina and
mistercrunch
replaced Raw with Union for uuid_or_id Missing import added marshmallow-union marshmallow union
dankor
force-pushed
the
uuid-get-dataset
branch
from
ab6e1d7 to
01b4ebb
Compare
| return query.filter(filter).one_or_none() | ||
| except StatementError: | ||
| # can happen if int is passed instead of a string or similar | ||
| return None |
There was a problem hiding this comment.
mmmh, shouldn't we either handle int or let it raise here? Not sure about what the implications are or might be...
There was a problem hiding this comment.
i guess mypy should complain if using an int anywhere in the codebase. Since this is a base function going to get reused it could be good to just support int here. Might even be a tiny bit cheaper to validate the type than isdigit() is, but would run both (pseudo: type(v) == int or isdigit())
There was a problem hiding this comment.
Well, you're right, the comment is a little bit irrelevant. Fixed.
Not sure about int optimization thought. Don't we plan to deprecate id eventually?
There was a problem hiding this comment.
I think you're right. Doesn't matter and mypy should catch error (would be good to confirm). While we're in here and setting the pattern, should we support short-shas in here too? Would make for nicer looking urls, pretty standard nowadays (git/github/...), not sure if it's worth the complexity, collision risks, ...
There was a problem hiding this comment.
Why is this useful? We don’t currently expose these URLs in the UI. Maybe in the next iteration we should reintroduce the slug + short id URL style (similar to Medium), for example:
- dashboard/unicode-test-91c65d59d37/
- chart/unicode-cloud-b86ba33fc5cd/
That said, this looks like a complex and potentially breaking change. It could be useful for dashboards, especially as a replacement for custom slugs. But charts and datasets don’t follow the same pattern — they just redirect you to the /explore endpoint.
I’d prefer to discuss this in a different setting, since it will require some brainstorming.
mistercrunch
left a comment
mistercrunch
left a comment
There was a problem hiding this comment.
added a minor comment but otherwise LGTM
(cherry picked from commit 077724c)
(cherry picked from commit 077724c)
(cherry picked from commit 077724c)
(cherry picked from commit 077724c)
2 participants
SUMMARY
It's a followup of #29573
Added uuid support:
uuid_or_iduuid_or_id/related_objectsuuid_or_id, "type": "table"}}BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Run the same request and replace id with it uuid.
ADDITIONAL INFORMATION